-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Clarify state attributes rules for entity #2437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds documentation to Changes
Sequence Diagram(s)No sequence diagram — documentation-only changes that do not alter control flow. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential review focus:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/core/entity.md (2)
122-132: Fix grammar and consider adding a code example.
- There's a grammatical error in the text. Please fix:
-If an attributes is not expected +If an attribute is not expected
- Consider adding a code example to illustrate these rules. For instance:
class MyEntity(Entity): @property def extra_state_attributes(self): """Return entity specific state attributes.""" attributes = { "expected_attribute": self._value or None, # Always present "configuration_dependent": "value" # Only when configured } return attributes🧰 Tools
🪛 LanguageTool
[grammar] ~131-~131: The plural noun “attributes” cannot be used with the article “an”. Did you mean “an attribute” or “attributes”?
Context: ...None` and remain in the dictionary - If an attributes is not expected, it should not be in th...(A_NNS)
Line range hint
134-139: Consider enhancing the System properties documentation.The section could be more helpful with additional context:
- Add a note explaining that these properties are managed through the entity registry.
- Add a warning about not implementing custom logic for these properties, as they are system-managed.
Example addition:
:::warning These properties are managed by Home Assistant through the entity registry. Integrations should not implement custom logic for these properties as it may conflict with system behavior. :::🧰 Tools
🪛 LanguageTool
[grammar] ~131-~131: The plural noun “attributes” cannot be used with the article “an”. Did you mean “an attribute” or “attributes”?
Context: ...None` and remain in the dictionary - If an attributes is not expected, it should not be in th...(A_NNS)
🪛 Markdownlint
120-120: Expected: 4; Actual: 3; Too few cells, row will be missing data
Table column count(MD056, table-column-count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/core/entity.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/core/entity.md
[grammar] ~131-~131: The plural noun “attributes” cannot be used with the article “an”. Did you mean “an attribute” or “attributes”?
Context: ...None` and remain in the dictionary - If an attributes is not expected, it should not be in th...
(A_NNS)
🔇 Additional comments (1)
docs/core/entity.md (1)
Line range hint 1-1000: Overall documentation changes look good!
The additions effectively clarify the rules for state attributes and system properties, which aligns well with the PR objectives. The document maintains good structure and readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~131-~131: The plural noun “attributes” cannot be used with the article “an”. Did you mean “an attribute” or “attributes”?
Context: ...None` and remain in the dictionary - If an attributes is not expected, it should not be in th...
(A_NNS)
🪛 Markdownlint
120-120: Expected: 4; Actual: 3; Too few cells, row will be missing data
Table column count
(MD056, table-column-count)
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/core/entity.md (2)
122-132: Enhance the state attributes section with examplesWhile the rules are clear, adding concrete examples would help developers better understand when attributes should be included or excluded from the dictionary. Consider adding examples for:
- Expected attributes (e.g., supported features)
- Attributes that should be None
- Attributes that should not be in the dictionary
Here's a suggested addition after line 132:
- If an attribute is not expected, it should not be in the dictionary. An attribute is expected if supported by the entity, for example as indicated by `supported_features`. + If an attribute is not expected, it should not be in the dictionary. An attribute is expected if supported by the entity, for example as indicated by `supported_features`. + + ### Examples + + ```python + @property + def extra_state_attributes(self): + """Return entity specific state attributes.""" + attributes = {} + + # Example 1: Expected attribute based on supported features + if self._attr_supported_features & SUPPORT_BRIGHTNESS: + attributes["brightness"] = self._brightness + + # Example 2: Expected attribute that might be None + if self._attr_supported_features & SUPPORT_COLOR: + attributes["rgb_color"] = self._rgb_color # Could be None + + # Example 3: Conditional attribute that should not be included + # Don't do this: + # if self._is_dimmable: + # attributes["brightness"] = self._brightness + + return attributes + ```
Line range hint
143-401: LGTM with a minor suggestionThe updated sections on entity naming and property implementation are clear and well-documented. The examples effectively demonstrate best practices. Consider adding a note about error handling in the property implementation example to make it more complete.
Add error handling to the example:
def update(self) -> None: """Update entity state.""" try: self._device.update() - except ExampleException: + except ExampleException as ex: if self.available: # Read current state, no need to prefix with _attr_ - LOGGER.warning("Update failed for %s", self.entity_id) + LOGGER.warning("Update failed for %s: %s", self.entity_id, str(ex)) self._attr_available = False # Set property value return + except Exception as ex: + LOGGER.error("Unexpected error updating %s: %s", self.entity_id, str(ex)) + self._attr_available = False + return🧰 Tools
🪛 Markdownlint
120-120: Expected: 4; Actual: 3; Too few cells, row will be missing data
Table column count(MD056, table-column-count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/core/entity.md(1 hunks)
🔇 Additional comments (1)
docs/core/entity.md (1)
Line range hint 134-141: LGTM!
The new "System properties" section clearly explains that these properties are controlled by Home Assistant and should not be overridden by integrations. The documentation for the enabled property is accurate and helpful.
🧰 Tools
🪛 Markdownlint
120-120: Expected: 4; Actual: 3; Too few cells, row will be missing data
Table column count
(MD056, table-column-count)
docs/core/entity.md
Outdated
| ## State attributes | ||
|
|
||
| Entities can provide additional state attributes besides the built-in ones, to provide further details to its state. | ||
| This should be done by providing a dictionary with keys and values to the `extra_state_attributes` property. | ||
|
|
||
| Providing state attributes comes with the following rules: | ||
|
|
||
| - If an attribute is expected, it should be in the dictionary. Attributes should not "come and go". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also indicate our rules for what belongs in state attributes and what should be separate entities and what should be action calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would probably be good although do we really have clear rules?
I think we could do that in a follow-up in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frenck once mentioned this in the members channel
Some helpful guidance with that, that I apply (for myself)
- Would someone extract it using a template into an other (binary) sensor -> Not an attribute
- It is essential to automation on? Can it be standalone as its own entity? -> Not an attribute
- It is static, e.g., from configuration; doesn't belong in the state machine -> Not an attribute
- Do we want to track long term stats of the value? -> Not an attribute
Additionally:
- Does is have historical value? If not, exlude it from being recorded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense but I'm not sure we can classify it as rules?
Maybe as "guidelines"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right. I think that if we are going to talk about state attributes, that we also explain that we are picky in what we want using these guidelines :)
|
|
||
| - If an attribute is expected, it should be in the dictionary. Attributes should not "come and go". | ||
| - If an attribute is expected, but it's not providing a value right now, its value should be `None` and remain in the dictionary. | ||
| - If an attribute is not expected, it should not be in the dictionary. An attribute is expected if supported by the entity, for example as indicated by `supported_features`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really sure what this means and how it would look in practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example, in statistics the buffer_usage_ratio attribute is only shown if the sensor is configured to handle a buffer size. Otherwise this attribute would be irrelevant hence not showing in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how can someone expect it to be there? We're quite bad with describing entities in the docs, let alone describing the state attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the developer docs not the user docs. The attribute is expected if it's part of a supported feature and that feature is supported or if it's an extra state attribute the attribute is expected if the device has support for that attribute.
docs/core/entity.md
Outdated
| Providing state attributes comes with the following rules: | ||
|
|
||
| - If an attribute is expected, it should be in the dictionary. Attributes should not "come and go". | ||
| - If an attribute is expected, but it's not providing a value right now, its value should be `None` and remain in the dictionary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the first 2 rules are quite similar. I also think we should explain what happens when for example the entity is unavailable. What should we do then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could perhaps combine the first two as they talk about the same thing.
I'm not sure what you mean by unavailable? Should there be any special consideration for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an entity is unavailable, does it still read the extra state attributes property when writing state? And if so, should everything be set to None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State attributes are only set if the state of the entity is not unavailable. This is part of the base entity logic. Integrations should not adjust any logic concerning availability and state attributes.
|
@gjohansson-ST Should we move this forward? |
|
Yes, I'll try to rewrite from the feedback shortly so we can merge it in. |
eabb8aa to
597eb89
Compare
|
Took a little bit of time but it's now rewritten according to the feedback so far. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
docs/core/entity.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/core/entity.md
[grammar] ~139-~139: Use a hyphen to join words.
Context: ...an attribute. - Do we want to track long term statistics of the value? -> Not an ...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - developers-home-assistant
- GitHub Check: Header rules - developers-home-assistant
- GitHub Check: Pages changed - developers-home-assistant
🔇 Additional comments (1)
docs/core/entity.md (1)
122-130: Verify content aligns with prior architectural feedback.The new section appropriately addresses prior review feedback by clarifying that rules apply to all state attributes, including the
supported_featuresexample for determining expected attributes. The guidelines section (lines 134–140) incorporates the decision criteria suggested in earlier reviews.One minor observation: Line 124 reads "to provide further details to its state"—the past review suggested "to provide further details of its state" but your wording is acceptable and natural. No change needed here.
Verify that the placement of this new section doesn't create any structural issues in the rendered documentation and that the guidelines provide sufficient clarity without needing inline examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
docs/core/entity.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/core/entity.md
[grammar] ~139-~139: Use a hyphen to join words.
Context: ...an attribute. - Do we want to track long term statistics of the value? -> Not an ...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - developers-home-assistant
- GitHub Check: Header rules - developers-home-assistant
- GitHub Check: Pages changed - developers-home-assistant
🔇 Additional comments (1)
docs/core/entity.md (1)
122-140: Duplication has been resolved—"State attributes" section appears only once.The verification confirms there is exactly one "## State attributes" section header in docs/core/entity.md. The section is not duplicated, and the provided code snippet accurately represents the current state of the document.
| ## State attributes | ||
|
|
||
| Entities can provide additional state attributes besides the built-in ones, to provide further details to its state. | ||
| This should be done by providing a dictionary with keys and values to the `extra_state_attributes` property. | ||
|
|
||
| Providing state attributes comes with the following rules: | ||
|
|
||
| - If an attribute is expected, it should be in the dictionary. Attributes should not "come and go". If it's not currently providing a value it should set it to `None`. | ||
| - If an attribute is not expected, it should not be in the dictionary. An attribute is expected if supported by the entity, for example as indicated by `supported_features`. | ||
|
|
||
| Most often, the use of state attributes should be limited and in general it's often better to use another sensor than providing data as attributes. | ||
|
|
||
| Some general guidance to consider how to handle it: | ||
|
|
||
| - Would someone extract it using a template into an other (binary) sensor -> Not an attribute. | ||
| - It is essential to automation on? Can it be standalone as its own entity? -> Not an attribute. | ||
| - It is static, e.g., from configuration; doesn't belong in the state machine -> Not an attribute. | ||
| - Do we want to track long term statistics of the value? -> Not an attribute. | ||
| - Does it have historical value? If not, it can be an attribute and exclude it from being recorded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content is well-structured and addresses the architecture decision. The new "State attributes" section clearly establishes rules and provides actionable guidance. However, there are several grammar and phrasing issues that need correction.
Apply the following diffs to fix grammar and phrasing issues:
- Would someone extract it using a template into an other (binary) sensor -> Not an attribute.
+ Would someone extract it using a template into another (binary) sensor -> Not an attribute.- It is essential to automation on? Can it be standalone as its own entity? -> Not an attribute.
+ Is it essential to automation? Can it be standalone as its own entity? -> Not an attribute.- Do we want to track long term statistics of the value? -> Not an attribute.
+ Do we want to track long-term statistics of the value? -> Not an attribute.- Does it have historical value? If not, it can be an attribute and exclude it from being recorded.
+ Does it have historical value? If not, it can be an attribute; exclude it from being recorded.🧰 Tools
🪛 LanguageTool
[grammar] ~139-~139: Use a hyphen to join words.
Context: ...an attribute. - Do we want to track long term statistics of the value? -> Not an ...
(QB_NEW_EN_HYPHEN)
Proposed change
Clarify the rules regarding additional state attributes as decided in home-assistant/architecture#680
Type of change
Additional information
Summary by CodeRabbit